Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

add init #1

Merged
merged 36 commits into from
May 17, 2021
Merged

add init #1

merged 36 commits into from
May 17, 2021

Conversation

BenjaminAbt
Copy link
Member

@BenjaminAbt BenjaminAbt commented May 15, 2021

  • init
  • browsers agents
  • bots agents
  • tools agents
  • nuget
  • docs
  • ci/cd
  • tests

@BenjaminAbt BenjaminAbt requested a review from gfoidl May 15, 2021 13:22
Copy link
Contributor

@gfoidl gfoidl left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍🏻 definitely a good starting-point, good architecture, and what I've seen it shouldn't allocate too much* 😃

Review comments got a bit longer, though / sorry.

.editorconfig should be added and these rules run through code cleanup (end of file line endings).

So I expect this is the most performant solution we can use, should go forward with it -- here I'm pretty confident 🚀

What I don't like, and at the moment I have no better solution, is that the HttpUserAgentParser needs to iterate and test (~ brute force). But this is simple and clean code which together with the CachedHttpUserAgentParserProvider shouldn't be executed that often, as the majority of UAs should be served from the cache.

* when used with the cache it should be allocation-free (iif cache-hit), that's something I like very much!


The rest of this comment are some thoughts, but these won't hinder any progress of this PR.

How will the data in HttpUserAgentStatics be updated / kept up-to-date?
I have no knowledge on how often UAs change or new bots come into play. But maybe we should have some sort of generator that creates the entries for us. Or we load at runtime (maybe periodically) UA strings and add them to the dictionary/list/array.

You know I love metrics and counters, so (in the future) we should add some counters for cache hit / miss and write the raw UA strings as events. So we can gather data for AI and analyze it.
So we could also detect new bots, etc. by an alert from AI and update the list of kwown UAs.

Copy link
Contributor

@gfoidl gfoidl left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

.editorconfig for consistent newlines would be nice 😉

Modulo these points - LGTM.
You can bring the change in (once these are addressed).
Some other points (regex compiled, dictionary) we can address later, as it's no stopper for this PR / project.

src/MyCSharp.HttpUserAgentParser/HttpUserAgentParser.cs Outdated Show resolved Hide resolved
src/MyCSharp.HttpUserAgentParser/HttpUserAgentParser.cs Outdated Show resolved Hide resolved
new(CreateDefaultPlatformRegex("symbian"), "Symbian OS", HttpUserAgentPlatformType.Symbian),
};

private const RegexOptions DefaultBrowserRegexFlags = RegexOptions.IgnoreCase | RegexOptions.Compiled;
Copy link
Contributor

@gfoidl gfoidl May 16, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just thinking about RegexOptions.Compiled.
If these regexes are only called if the UA isn't in the cache, is it worth then to have compiled regexes?
Compiled regex have higher startup-cost (won't matter much for us, but matter e.g. for Azure Functions) and need to be kept around / alive for the lifetime of the app. They are only ever re-executed once the UA is evicted from the cache (e..g by size limit).

I don't know what's better here. For the moment we should leave it as is, and should re-evaluate that later (--> #3).

src/MyCSharp.HttpUserAgentParser/HttpUserAgentStatics.cs Outdated Show resolved Hide resolved
Copy link
Contributor

@gfoidl gfoidl left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wow, I'm pedantic (sorry...).

README.md Outdated Show resolved Hide resolved
README.md Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
@gfoidl
Copy link
Contributor

gfoidl commented May 17, 2021

Lets bring the PR in, do the rest in further PRs to keep it manageable.

@BenjaminAbt BenjaminAbt requested a review from gfoidl May 17, 2021 13:48
Copy link
Contributor

@gfoidl gfoidl left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd bring the benchmarks in as separate change.
A PR more won't harm the project, and the changes are separated cleaner.
We also have some follow up work to do.
Otherwise we have monster PRs with lots of comments and orthogonal change-sets.

Directory.Build.props Show resolved Hide resolved
@BenjaminAbt BenjaminAbt merged commit 1236675 into main May 17, 2021
@BenjaminAbt BenjaminAbt deleted the feature/init branch May 17, 2021 15:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants